Fenrir fixes#10968
Open
Frauschi wants to merge 8 commits into
Open
Conversation
…nal opt-in knobs These compile-time flags are off by default and RFC-non-conformant when enabled. Document their scope and intent at the asn.c macro list and each enforcement site (ParseCertRelative, VerifyCRL_Signature, AddCA, ProcessPeerCerts) so their opt-in nature is clear. No behavior change. Fixes F-6984 and F-6985.
In the WOLF_CRYPTO_CB_ONLY_ECC path of wolfSSL_ECDSA_do_verify, a non-positive length from i2d_ECDSA_SIG (an encoding failure) skipped the verify block and left ret at its initial value of 1, reporting a valid signature although no verification was performed. Map the encoding failure to WOLFSSL_FATAL_ERROR so it is not treated as a successful verification. Fixes F-6981.
VerifyX509Acert parsed the acert and checked the signature but never validated the notBefore and notAfter dates, so wolfSSL_X509_ACERT_verify and wc_VerifyX509Acert accepted expired or not-yet-valid attribute certificates whenever the signature was good. Call CheckDate for both validity bounds before signature verification. CheckDate returns the proper date error and honors the runtime skip-date control. Also correct ParseX509Acert to report ASN_AFTER_DATE_E instead of ASN_BEFORE_DATE_E when the notAfter date check fails. Fixes F-6986.
The sniffer elided all session, server, and keylog secret list locking whenever HAVE_C___ATOMIC was defined, assuming those tables are thread-local and need no mutex. Thread-locality is actually governed by THREAD_LS_T, which is only a real qualifier when HAVE_THREAD_LS is set and NO_THREAD_LS is not. A build with HAVE_C___ATOMIC and NO_THREAD_LS left the tables as process-shared globals with no synchronization, so concurrent packet processing raced on the list pointers and could free a session another thread was still using. Gate the lock elision on both the atomic fast path and real thread-local storage, and take the mutexes whenever the tables are process-shared. The stats counter path is unchanged because it uses a genuine atomic add. Fixes F-6983.
InitSuitesHashSigAlgo added the ecdsa_sha1 and rsa_pkcs1_sha1 signature schemes to the signature_algorithms list based only on the build flags, ignoring the negotiated protocol version. Because that list is also the set a peer's signatures are validated against, any build with old TLS compiled in advertised and accepted SHA-1 handshake and certificate signatures for TLS 1.2, which RFC 9155 deprecates. Gate the SHA-1 schemes on the negotiated version so they are offered only for TLS 1.0 and 1.1 handshakes, unless WOLFSSL_ALLOW_TLS_SHA1 is defined to opt back in. The same gate excludes them for TLS 1.3, as required by RFC 8446. Fixes F-6991.
The SecureRenegotiation extension embeds a Keys tmp_keys copy of the session cipher and MAC keys, which are the keys used for the renegotiated epoch. SCR_FREE_ALL freed that struct with a bare XFREE, leaving a full set of session keys intact in freed heap memory. Wipe the struct with ForceZero before freeing it, matching the ForceZero of ssl->keys on connection teardown. Fixes F-7008.
WOLFSSL_ALPN_CONTINUE_ON_MISMATCH makes a server continue the handshake without an agreed protocol when no ALPN protocol matches, like OpenSSL, instead of sending the fatal no_application_protocol alert that RFC 7301 section 3.2 requires. This is an explicit, caller-selected opt-in: wolfSSL_UseALPN rejects a call that sets neither mismatch option, and the library never enables it implicitly. Document the RFC non-compliance at the option enum, the wolfSSL_UseALPN options parameter, and the mismatch handling in ALPN_find_match so the trade-off is clear at every point a user or maintainer encounters it. No behavior change. Fixes F-7003.
RFC 9147 section 5.6.1 states that EndOfEarlyData is not used in DTLS 1.3 and that a receiver must terminate the connection with an unexpected_message alert. Dtls13CheckEpoch grouped end_of_early_data into the default case that returns SANITY_MSG_E without sending any alert, and the DTLS 1.3 handshake dispatch in DoProcessReplyEx did not send a fatal alert on error the way the DTLS 1.2 path does, so the connection was dropped silently. Add an explicit end_of_early_data case that sends the unexpected_message alert, and mirror the DTLS 1.2 SendFatalAlertOnly handling in the DTLS 1.3 dispatch so other handshake errors are also reported rather than dropped silently. Fixes F-6987.
|
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10968
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
No new issues found in the changed files. ✅
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch collects a set of security-hardening and RFC-conformance fixes that a Fenrir security review surfaced across wolfCrypt (ASN.1 / attribute certificates, ECC), the TLS and DTLS 1.3 stacks, the OpenSSL compatibility layer, and the sniffer. It also documents three intentional, off-by-default build knobs whose RFC non-conformance was previously undocumented.
Behavioral fixes
wolfSSL_ECDSA_do_verifyreturned success (fail-open) in theWOLF_CRYPTO_CB_ONLY_ECCpath wheni2d_ECDSA_SIGfailed to encode the signature. It now maps the encoding failure toWOLFSSL_FATAL_ERRORso an error is never reported as a valid signature.VerifyX509Acert(and thereforewc_VerifyX509Acert/wolfSSL_X509_ACERT_verify) never checked the certificate validity period, so an expired or not-yet-valid attribute certificate verified successfully. It now enforcesnotBefore/notAfterviaCheckDate. Also correctsParseX509Acertto reportASN_AFTER_DATE_Einstead ofASN_BEFORE_DATE_Eon a failednotAftercheck.HAVE_C___ATOMICwas defined, on the assumption the tables are thread-local. Thread-locality is actually governed byTHREAD_LS_T, so a build withHAVE_C___ATOMICandNO_THREAD_LSleft the tables as process-shared globals with no synchronization (data race plus use-after-free during renegotiation). Lock elision is now gated on real thread-local storage.signature_algorithmslist advertised and accepted the SHA-1 schemesrsa_pkcs1_sha1andecdsa_sha1for TLS 1.2 and later, based only on build flags. They are now offered only for TLS 1.0 / 1.1 handshakes unlessWOLFSSL_ALLOW_TLS_SHA1is defined.SecureRenegotiationextension embeds aKeys tmp_keyscopy of the session cipher and MAC keys used for the renegotiated epoch. It was freed with a bareXFREE, leaving key material in freed heap. It is now wiped withForceZerobefore free, matching thessl->keysteardown.EndOfEarlyDatamessage did not send the requiredunexpected_messagefatal alert; the connection was dropped silently.Dtls13CheckEpochnow sends the alert, and the DTLS 1.3 handshake dispatch inDoProcessReplyExnow callsSendFatalAlertOnlyon error, mirroring the DTLS 1.2 path so other handshake errors are reported rather than dropped silently.Documentation (intentional opt-in behaviour, no code change)
WOLFSSL_ALPN_CONTINUE_ON_MISMATCHmakes a server continue without an agreed protocol on mismatch, like OpenSSL, instead of sending theno_application_protocolalert. This is an explicit, caller-selected opt-in (wolfSSL_UseALPNrejects a call that sets neither mismatch option). The RFC non-conformance is now documented at the option enum, thewolfSSL_UseALPNparameter docs, and theALPN_find_matchenforcement site.ALLOW_INVALID_CERTSIGNandIGNORE_KEY_EXTENSIONSare off-by-default, RFC-non-conformant escape hatches for interop with deployed certificates that carry malformed key usage. Their scope and intent are now documented at theasn.cmacro list and each enforcement site.Testing
New and extended regression tests:
tests/api/test_ossl_x509_acert.c- extendstest_wolfSSL_X509_ACERT_verifyto install awc_SetTimeCbthat moves the clock past the test certificate'snotAfter, then asserts verification fails. Fails before the attribute-certificate fix, passes after.tests/api.c- extendstest_wolfSSL_sigalg_infoto assert thesignature_algorithmslist contains no SHA-1 scheme for TLS 1.2 and TLS 1.3 (tls1_2set), and still contains them for TLS 1.0 / 1.1 when compiled in. Fails before the SHA-1 gating fix, passes after.tests/api/test_dtls13.c- extendstest_dtls13_epochsto assert a fatalunexpected_messagealert is recorded afterDtls13CheckEpoch(ssl, end_of_early_data). Fails before the DTLS 1.3 fix, passes after.Fixes without a dedicated test, with rationale:
WOLF_CRYPTO_CB_ONLY_ECCand needs fault injection to force an encoding failure. Verified by compiling that configuration (with a negative-control syntax error to prove the branch is built) and running the ECC and OpenSSL-compat EC suites.NO_THREAD_LS, and runningscripts/sniffer-testsuite.testin both.--enable-secure-renegotiationwith AddressSanitizer and confirming the free path runs cleanly (0 sanitizer reports) across the secure-renegotiation tests.All changes verified by building the affected feature configuration, running the touched suite (full API suite reports 0 failures in each configuration), and restoring the working configuration afterward.